Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: fix Bazel 0.20.0 compatibility issues #1661

Merged
merged 10 commits into from
Dec 6, 2018

Conversation

wchargin
Copy link
Contributor

@wchargin wchargin commented Dec 5, 2018

Summary:
Bazel 0.20.0 introduced some incompatible changes that broke both our
code and our dependencies. Our dependencies also introduced some
incompatible changes of their own. This commit upgrades our dependencies
and fixes up our code to accommodate them.

An upgrade to transitive dependency rules_go introduces a dependency
on the @bazel_tools//tools/build_defs/cc:action_names.bzl builtin
target. This target was only introduced in Bazel 0.16.0, so as a
consequence our workspace is no longer compatible with earlier versions
of Bazel. In practice, we must use at least version 0.16.1 to fix a
regression in 0.16.0: bazelbuild/bazel#5726.

Unfortunately, Bazel 0.16.0 also introduced a regression that prevents
us from using the --distinct_host_configuration=false option; this has
not been fixed as of Bazel 0.20.0. As a result, parts of our build will
now be twice as slow.

The new urllib3 dependency is due to a transitive Selenium upgrade.

This commit shifts our Bazel version window from [0.13.0, 0.19.2] to
[0.16.1, 0.20.0].

Test Plan:
Running bazel test //tensorboard/... passes in Python 2 and 3 in Bazel
versions 0.16.1 and 0.20.0 (latest at time of writing), with
the .bazelrc file set up as on Travis:

>~/.bazelrc
echo "startup --output_base=${HOME}/.bazel-output-base" >>~/.bazelrc
echo "startup --host_jvm_args=-Xms500m" >>~/.bazelrc
echo "startup --host_jvm_args=-Xmx500m" >>~/.bazelrc
echo "startup --host_jvm_args=-XX:-UseParallelGC" >>~/.bazelrc
echo "build --local_resources=400,2,1.0" >>~/.bazelrc
echo "build --worker_max_instances=2" >>~/.bazelrc
echo "build --worker_verbose" >>~/.bazelrc
echo "build --worker_sandboxing" >>~/.bazelrc
echo "build --spawn_strategy=sandboxed" >>~/.bazelrc
echo "build --genrule_strategy=sandboxed" >>~/.bazelrc
echo "test --test_verbose_timeout_warnings" >>~/.bazelrc
echo "build --verbose_failures" >>~/.bazelrc
echo "test --test_output=errors" >>~/.bazelrc

wchargin-branch: bazel-0.20.0-fixes

Summary:
Bazel 0.20.0 introduced some incompatible changes that broke both our
code and our dependencies. Our dependencies also introduced some
incompatible changes of their own. This commit upgrades our dependencies
and fixes up our code to accommodate them.

One of the changes is the removal of `native.[new_]http_archive`, which
is replaced by a rule to be loaded from the `@bazel_tools` namespace.
This rule was only introduced in Bazel 0.16.0, so as a consequence our
workspace is no longer compatible with earlier versions of Bazel.

The new `urllib3` dependency is due to a transitive Selenium upgrade.

This commit shifts our Bazel version window from [0.13.0, 0.19.2] to
[0.16.0, 0.20.0].

Test Plan:
Running `bazel test //tensorboard/...` passes in Python 2 and 3 in Bazel
versions 0.16.0, 0.19.2, and 0.20.0 (latest at time of writing).

wchargin-branch: bazel-0.20.0-fixes
@wchargin
Copy link
Contributor Author

wchargin commented Dec 5, 2018

Travis failure is:

The command "wget -t 3 -O bazel https://mirror.bazel.build/github.com/bazelbuild/bazel/releases/download/${BAZEL}/bazel-${BAZEL}-linux-x86_64" failed and exited with 8 during .

I'll figure out how to mirror this.

@wchargin
Copy link
Contributor Author

wchargin commented Dec 5, 2018

Uploaded Bazel to mirror; restarting build.

Copy link
Contributor

@nfelt nfelt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for figuring this all out! How hard would it be to add the actual github release URL for bazel as fallback call to wget in travis.yml? That way we aren't solely dependent on the mirror.

.travis.yml Outdated Show resolved Hide resolved
@wchargin
Copy link
Contributor Author

wchargin commented Dec 6, 2018

The Travis failure, which I could not reproduce locally with the same
version of Bazel, appears to be due to a bug in Bazel 0.16.0 that
introduces an implicit dependency on the native Java runtime. Upgrading
to Bazel≥0.16.1 solves this issue.

It looks like there may be more issues to track down; I’ll investigate
further tomorrow.

wchargin-branch: bazel-0.20.0-fixes
wchargin-branch: bazel-0.20.0-fixes
WORKSPACE Outdated Show resolved Hide resolved
wchargin-branch: bazel-0.20.0-fixes
wchargin-branch: bazel-0.20.0-fixes
wchargin-branch: bazel-0.20.0-fixes
wchargin-branch: bazel-0.20.0-fixes
@wchargin
Copy link
Contributor Author

wchargin commented Dec 6, 2018

Thanks so much for figuring this all out! How hard would it be to add
the actual github release URL for bazel as fallback call to wget in
travis.yml? That way we aren't solely dependent on the mirror.

This sounds like a good idea. If you don’t mind, I’ll do it in a
follow-up PR.

(Edit: I didn’t answer your question. It’s not hard.)

wchargin-branch: bazel-0.20.0-fixes
@wchargin
Copy link
Contributor Author

wchargin commented Dec 6, 2018

@nfelt: Does this PR still have your approval?

Copy link
Contributor

@nfelt nfelt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

.travis.yml Outdated Show resolved Hide resolved
.travis.yml Show resolved Hide resolved
wchargin-branch: bazel-0.20.0-fixes
wchargin-branch: bazel-0.20.0-fixes
@wchargin wchargin merged commit cdfcb45 into master Dec 6, 2018
@wchargin wchargin deleted the wchargin-bazel-0.20.0-fixes branch December 6, 2018 22:39
wchargin added a commit that referenced this pull request Dec 6, 2018
Summary:
As suggested by @nfelt in:
<#1661 (review)>

Test Plan:
Verify that the new chunk of shell script has the following properties:

  - When executed with the environment variables at the top of the
    Travis config, successfully downloads Bazel 0.16.1 and returns 0.
  - When executed with `BAZEL=0.19.2` (which does not exist in the
    mirror) and shasum updated per the instructions in the comment,
    successfully downloads Bazel 0.19.2 and returns 0.
  - When executed with `BAZEL=0.wat.0` (which does not exist), fails
    and returns 1.
  - When a bit is flipped in the shasum, fails and returns 1, having
    removed the invalid downloads.

Note that Shellcheck reports no errors or warnings with `#!/bin/sh`.

wchargin-branch: travis-bazel-download-github-fallback
wchargin added a commit that referenced this pull request Dec 6, 2018
Summary:
As suggested by @nfelt in:
<#1661 (review)>

Test Plan:
Verify that the new chunk of shell script has the following properties:

  - When executed with the environment variables at the top of the
    Travis config, successfully downloads Bazel 0.16.1 and returns 0.
  - When executed with `BAZEL=0.19.2` (which does not exist in the
    mirror) and shasum updated per the instructions in the comment,
    successfully downloads Bazel 0.19.2 and returns 0.
  - When executed with `BAZEL=0.wat.0` (which does not exist), fails
    and returns 1.
  - When a bit is flipped in the shasum, fails and returns 1, having
    removed the invalid downloads.

Note that Shellcheck reports no errors or warnings with `#!/bin/sh`.

wchargin-branch: travis-bazel-download-github-fallback
stephanwlee added a commit to stephanwlee/tensorboard-plugin-example that referenced this pull request Dec 17, 2018
This is mainly copy-paste from TensorBoard repo.
Please see the PR for more details.
tensorflow/tensorboard#1661
stephanwlee added a commit to tensorflow/tensorboard-plugin-example that referenced this pull request Dec 18, 2018
This is mainly copy-paste from TensorBoard repo.
Please see the PR for more details.
tensorflow/tensorboard#1661
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants